Support for overriding OpenSSL ciphers #3442
Conversation
| if ciphers_string: | ||
| self._ssl_ciphers = AcceptableCiphers.fromOpenSSLCipherString(ciphers_string) | ||
| else: | ||
| self._ssl_ciphers = DEFAULT_CIPHERS |
There was a problem hiding this comment.
DEFAULT_CIPHERS is a singleton so I kept it as it maybe improves the performance.
There was a problem hiding this comment.
I'm not sure it helps, as download handler always pass ciphers_string, which is "DEFAULT" by default.
tests/mockserver.py
Outdated
| def broken_ssl_context_factory(keyfile='keys/localhost.key', certfile='keys/localhost.crt', cipher_string='DEFAULT'): | ||
| factory = ssl_context_factory(keyfile, certfile) | ||
| ctx = factory.getContext() | ||
| ctx.set_options(SSL.OP_CIPHER_SERVER_PREFERENCE | SSL.OP_NO_TLSv1_2) |
There was a problem hiding this comment.
TLS 1.2 has it's own always enabled ciphers so I disable it.
Twisted recommends using CertificateOptions instead of DefaultOpenSSLContextFactory, and its API is cleaner, but it requires file-like objects for certs and also I don't know if all supported Twisted versions support this API.
There was a problem hiding this comment.
CertificateOptions was there at least in twisted 9.0.0 (https://twistedmatrix.com/documents/9.0.0/api/twisted.internet.ssl.CertificateOptions.html), so it should be fine to use. But I have no idea what I'm talking about, or what is better in this particular case :)
tests/mockserver.py
Outdated
| @@ -222,6 +224,14 @@ def ssl_context_factory(keyfile='keys/localhost.key', certfile='keys/localhost.c | |||
| ) | |||
|
|
|||
|
|
|||
| def broken_ssl_context_factory(keyfile='keys/localhost.key', certfile='keys/localhost.crt', cipher_string='DEFAULT'): | |||
There was a problem hiding this comment.
Could you please explain what this code does? Does it 1) add cipher_string support, and 2) disables default ciphers which come from TLSv1_2? And the purpose is to create servers which use outdated ciphers not supported by current http clients by default?
If so, what do you think about
- removing this function,
- adding cipher_string argument to ssl_context_factory, None by default,
- when cipher_string is not None, ssl_context_factory becomes broken_ssl_context_factory,
- add a comment which explains why
ctx.set_options(SSL.OP_CIPHER_SERVER_PREFERENCE | SSL.OP_NO_TLSv1_2)line exists.
tests/test_webclient.py
Outdated
| self.assertEqual, to_bytes(s)) | ||
|
|
||
|
|
||
| class WebClientBrokenSSLTestCase(WebClientSSLTestCase): |
There was a problem hiding this comment.
BrokenSSL -> OldCiphers / InsecureCIphers?
Codecov Report
@@ Coverage Diff @@
## master #3442 +/- ##
==========================================
- Coverage 85.35% 85.22% -0.14%
==========================================
Files 167 167
Lines 9699 9794 +95
Branches 1453 1478 +25
==========================================
+ Hits 8279 8347 +68
- Misses 1162 1185 +23
- Partials 258 262 +4
|
|
Awesome, this is a much-needed feature which may solve quite a lot of issues in Scrapy bug tracker 👍 |
tests/mockserver.py
Outdated
| @@ -3,6 +3,8 @@ | |||
| from six.moves.urllib.parse import urlencode | |||
| from subprocess import Popen, PIPE | |||
|
|
|||
| from OpenSSL import SSL | |||
|
|
|||
There was a problem hiding this comment.
a nitpick: this import should be with twisted imports according to pep8 (3 groups: stdlib imports, third-party imports, internal imports)
| interface=self.host) | ||
| self.portno = self.port.getHost().port | ||
| self.download_handler = self.download_handler_cls( | ||
| Settings({'DOWNLOADER_CLIENT_TLS_CIPHERS': 'CAMELLIA256-SHA'})) |
There was a problem hiding this comment.
could you please also add a test which shows that downloading doesn't work with default DOWNLOADER_CLIENT_TLS_CIPHERS, to make sure the other test is valid?
wRAR
changed the title
wRAR
changed the title
|
The tests fail on the "jessie" env, I tried different pyOpenSSL and cryptography versions and found that the tests only work on cryptography 2.0+ (IIRC). Though I don't know why the Travis test failed with |
|
@wRAR could you please check what cryptography versions are available in Debians and Ubuntus we care about? |
|
@kmike We currently don't set the version constraints for pyOpenSSL or cryptography on envs other than jessie (and trusty, which we don't test anymore). Note that cryptography 2.0 only works with pyOpenSSL 16.2+. The failing test expects a connection error with a non-default server cipher and the default client setting but the connection works on these older libraries. I didn't see anything in the cryptography 2.0 changelog and didn't try to investigate further. |
|
Thanks @wRAR! |
This adds the
DOWNLOADER_CLIENT_TLS_CIPHERSsetting which is then used inScrapyClientContextFactory. There is a real-world example of when it may be needed in #3392 (the server offers a cipher that is rejected by the client so we need to not advertise it), there are other theoretical problems e.g. a server supports only ciphers that are supported by the client but disabled. I've constructed the second case in the tests because constructing the first case seems to be impossible with a modern OpenSSL.There are two tests, the one in
tests/test_webclient.pyusesScrapyClientContextFactorydirectly (its base class also just tests the default context factory with the default SSL server) and the one intests/test_downloader_handlers.pyuses the setting and the download handler. I don't know if the first one is worth keeping. I also don't like the amount of duplicated code in the added test cases, maybe some reshuffling of base HTTP(S) test classes will be useful.